deps: backport 9da3ab6 from V8 upstream #3609
Conversation
|
This seems to be a feature addition, adding semver-minor |
indutny
added
semver-minor
|
cc @nodejs/lts |
|
As a semver-minor it cannot land in v4.x. |
|
This is an extremely important, yet low risk feature for most enterprise customers. This effectively means that we're able to perform production CPU profiling at any time. It seems silly to have to wait basically until October 2016 for 6.0 to land for this feature to show up in an LTS supported release. |
|
I get that but similar arguments could be made for any semver-minor commit That said, the LTS WG and CTC can look at individual commits to see if it's |
|
|
|
I agree with @jasnell. Landing this in the LTS branch would set a bad precedent. |
|
Saying No to merging here is the right thing to do and important moving forward so we don't end up on a slippery slope. I think the comment (ok, both comments) from @jasnell perfectly explains this. |
I propose we evaluate this PR on merit instead:
I haven't seen an argument against this PR on merit yet. Remember that LTS is not about 'no changes'. It is about managing risk while still fixing real problems that people have. |
|
I've got no problem at all with evaluating this PR we just need to be
|
Why not evaluate each PR on its own merits? Why compare them to other PRs? |
It seems like you're focussing on syntax instead of the semantics. |
|
Not so much. There's no particular rush on getting this into v4 is there? I
|
|
@jasnell I can't overstate this enough, but this is a huge deal for us here at Netflix, and I dare say for other large enterprise customers as well. I recall that there were engineers from Uber and Peer5 on the original v8 issue (https://code.google.com/p/v8/issues/detail?id=3453) that also wanted to see this fixed in v8. Maybe @mranney can chime in here with Uber's perspective. Not sure how many folks here are running large Node production deployments in mission critical paths, but without a way to quickly and easily observe and profile Node application performance in production on Linux means that this is literally stifling Node adoption both internally at Netflix and probably externally as well. Technically, this may be a new feature -- but it is really a v8 bug fix. The v8 team has done a great job getting this bug fixed, and has done so for some time. Indeed I seem to recall that this patch landed in v8 (https://chromium.googlesource.com/v8/v8.git/+/9da3ab661fe7190fcb99bd99db30cf95913d3659) before node v4 was launched. Will we have to wait ~10 months before we're able to get this feature in an LTS release? That's 10 months of not having performance visibility into our critical Node applications which serve ~70 million subscribers. I'm happy to chat more on Monday at the WG meeting -- but I thought I'd share Netflix's perspective ahead of it. |
|
@yunong have you considered floating it on top of a LTS release if its that significant? Just saying that your agenda might be different than the LTS agenda in more than one scenario. |
|
Just reading the v4 release notes, it seems that from |
|
I get that this is important but one LGTM from me is not enough and there's
|
|
Should we consider the addition of a flag of a dependency a minor bump? Personally don't feel like this qualifies as a minor in the usual sense, and am fine landing it on LTS. LGTM |
|
The main point of sticking to the patch-level changes is that the risk is decreased. Adding features necessarily involves making the kind of changes that can lead to bugs and regressions. This kind of assessment has to be applied to patch-level changes too—the more they mess with internals the more risk they carry and the less they qualify for LTS. A flag for enabling certain things that would otherwise not make it in doesn't help any of this. |
Except this flag inhibits functionality. The code change is very straightforward, as it only acts like a filter for output of a existing flag. Another reason why I consider this more a patch level change. |
|
This is certainly a very useful thing, but I don't think it's important enough to enough users to warrant deviating from the release schedule. This is sad, but IMO is a reasonable consequence of a predictable release schedule and process. At Uber we don't depend on perf at all, and we are also not on node 4. I think big operations that actually engage in performance profiling will have enough sophistication to float this patch, and they probably already have a custom build process for their version of node anyway. |
|
Flagging this for We've agreed to land this in v4.x and know that the messaging is going to be really tough re versioning ("hey, this is supposed to be stable, why are you adding features?", "I thought you said it'll likely be 4.2.x for most of its life but now it's 4.3??") and also the fact that we've decided to deal with these on a case-by-case basis and there will be other items come up that people will want in LTS ("but you took #3609!"). We've had a number of discussions in the past about how our dependencies impact semver but we don't have a strict policy that would help us decide on this item. It's difficult to make a case that this is not a semver for V8 and it impacts on our CLI options because we expose V8 CLI options, is that enough to force semver-minor or can we argue this is a semver-patch because it's not a Node.js feature addition? |
|
@jasnell Thanks for the update and all the hard work 👍 |
MylesBorins
force-pushed
the
v4.x-staging
branch
from
3281c58
to
1ea40ac
Compare
MylesBorins
force-pushed
the
v4.x-staging
branch
from
8b08780
to
524b083
Compare
This patch exposes a new flag perf_basic_prof_only_functions (disabled by default) that can be useful for the use-case of running always-on profiling on long running production jobs. Original commit v8/v8@9da3ab6 New flag --perf_basic_prof_only_functions Restricts linux perf-event code range reporting to functions only (i.e. on stubs.) While this makes the gathered ticks less accurate, it reduces the growth of the /tmp/perf-${pid}.map file. BUG=v8:3453 [email protected],[email protected] LOG=N Review URL: https://codereview.chromium.org/1292743002 Cr-Commit-Position: refs/heads/master@{#30179} PR-URL: #3609 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trevor Norris <[email protected]>
|
landed in |
This patch exposes a new flag perf_basic_prof_only_functions (disabled by default) that can be useful for the use-case of running always-on profiling on long running production jobs. Original commit v8/v8@9da3ab6 New flag --perf_basic_prof_only_functions Restricts linux perf-event code range reporting to functions only (i.e. on stubs.) While this makes the gathered ticks less accurate, it reduces the growth of the /tmp/perf-${pid}.map file. BUG=v8:3453 [email protected],[email protected] LOG=N Review URL: https://codereview.chromium.org/1292743002 Cr-Commit-Position: refs/heads/master@{#30179} PR-URL: #3609 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trevor Norris <[email protected]>
Notable changes:
This update to the LTS line includes a number of semver minor changes
that have been staged for a number of months. This includes:
* deps: backport 9da3ab6 from V8 upstream (Ali Ijaz Sheikh)
- #3609
* http: handle errors on idle sockets (José F. Romaniello)
- #4482
* src: add BE support to StringBytes::Encode() (Bryon Leung)
- #3410
* tls: add `options` argument to createSecurePair (Коренберг Марк)
- #2441
There are also quite a large number of semver patch changes
including over 20 doc fixes and almost 50 test fixes.
Notable semver patch changes include:
* deps: upgrade to npm 2.14.18 (Kat Marchán)
- #5245
* https: evict cached sessions on error (Fedor Indutny)
- #4982
* process: support symbol events (cjihrig)
- #4798
* querystring: improve parse() performance (Brian White)
- #4675
PR-URL: #5301
In December we announced that we would be doing a minor release in
order to get a number of voted on SEMVER-MINOR changes into LTS.
Our ability to release this was delayed due to the unforeseen security
release v4.3. We are quickly bumping to v4.4 in order to bring you the
features that we had committed to releasing.
Notable changes:
The SEMVER-MINOR changes include:
* **deps**:
- An update to v8 that introduces a new flag
--perf_basic_prof_only_functions (Ali Ijaz Sheikh)
#3609
* **http**:
- A new feature in http(s) agent that catches errors on
*keep alived* connections (José F. Romaniello)
#4482
* **src**:
- Better support for Big-Endian systems (Bryon Leung)
#3410
* **tls**:
- A new feature that allows you to pass common SSL options to
`tls.createSecurePair` (Коренберг Марк)
#2441
Notable semver patch changes include:
* **npm**
- upgrade to npm 2.14.19 (Kat Marchán)
#5335
* **https**:
- A potential fix for #3692)
HTTP/HTTPS client requests throwing EPROTO (Fedor Indutny)
#4982
* **process**:
- Add support for symbols in event emitters. Symbols didn't exist
when it was written ¯\_(ツ)_/¯ (cjihrig)
#4798
* **querystring**:
- querystring.parse() is now 13-22% faster! (Brian White)
#4675
PR-URL: #5301
In December we announced that we would be doing a minor release in order to get a number of voted on SEMVER-MINOR changes into LTS. Our ability to release this was delayed due to the unforeseen security release v4.3. We are quickly bumping to v4.4 in order to bring you the features that we had committed to releasing. This release also includes security updates to openssl. More information can be found [on nodejs.org](https://nodejs.org/en/blog/vulnerability/openssl-march-2016/) This release also includes over 70 fixes to our docs and over 50 fixes to tests. The SEMVER-MINOR changes include: * deps: - An update to v8 that introduces a new flag --perf_basic_prof_only_functions (Ali Ijaz Sheikh) #3609 * http: - A new feature in http(s) agent that catches errors on *keep alived* connections (José F. Romaniello) #4482 * src: - Better support for Big-Endian systems (Bryon Leung) #3410 * tls: - A new feature that allows you to pass common SSL options to `tls.createSecurePair` (Коренберг Марк) #2441 * tools - a new flag `--prof-process` which will execute the tick processor on the provided isolate files (Matt Loring) #4021 Notable semver patch changes include: * buld: - Support python path that includes spaces. This should be of particular interest to our Windows users who may have python living in `c:/Program Files` (Felix Becker) #4841 * https: - A potential fix for #3692 HTTP/HTTPS client requests throwing EPROTO (Fedor Indutny) #4982 * installer: - More readable profiling information from isolate tick logs (Matt Loring) #3032 * *npm: - upgrade to npm 2.14.20 (Kat Marchán) #5510 * *openssl: - upgrade openssl to 1.0.2g (Ben Noordhuis) #5507 * process: - Add support for symbols in event emitters. Symbols didn't exist when it was written ¯\_(ツ)_/¯ (cjihrig) #4798 * querystring: - querystring.parse() is now 13-22% faster! (Brian White) #4675 * streams: - performance improvements for moving small buffers that shows a 5% throughput gain. IoT projects have been seen to be as much as 10% faster with this change! (Matteo Collina) #4354 * tools: - eslint has been updated to version 2.1.0 (Rich Trott) #5214 PR-URL: #5301
In December we announced that we would be doing a minor release in order to get a number of voted on SEMVER-MINOR changes into LTS. Our ability to release this was delayed due to the unforeseen security release v4.3. We are quickly bumping to v4.4 in order to bring you the features that we had committed to releasing. This release also includes security updates to openssl. More information can be found [on nodejs.org](https://nodejs.org/en/blog/vulnerability/openssl-march-2016/) This release also includes over 70 fixes to our docs and over 50 fixes to tests. The SEMVER-MINOR changes include: * deps: - An update to v8 that introduces a new flag --perf_basic_prof_only_functions (Ali Ijaz Sheikh) nodejs#3609 * http: - A new feature in http(s) agent that catches errors on *keep alived* connections (José F. Romaniello) nodejs#4482 * src: - Better support for Big-Endian systems (Bryon Leung) nodejs#3410 * tls: - A new feature that allows you to pass common SSL options to `tls.createSecurePair` (Коренберг Марк) nodejs#2441 * tools - a new flag `--prof-process` which will execute the tick processor on the provided isolate files (Matt Loring) nodejs#4021 Notable semver patch changes include: * buld: - Support python path that includes spaces. This should be of particular interest to our Windows users who may have python living in `c:/Program Files` (Felix Becker) nodejs#4841 * https: - A potential fix for nodejs#3692 HTTP/HTTPS client requests throwing EPROTO (Fedor Indutny) nodejs#4982 * installer: - More readable profiling information from isolate tick logs (Matt Loring) nodejs#3032 * *npm: - upgrade to npm 2.14.20 (Kat Marchán) nodejs#5510 * *openssl: - upgrade openssl to 1.0.2g (Ben Noordhuis) nodejs#5507 * process: - Add support for symbols in event emitters. Symbols didn't exist when it was written ¯\_(ツ)_/¯ (cjihrig) nodejs#4798 * querystring: - querystring.parse() is now 13-22% faster! (Brian White) nodejs#4675 * streams: - performance improvements for moving small buffers that shows a 5% throughput gain. IoT projects have been seen to be as much as 10% faster with this change! (Matteo Collina) nodejs#4354 * tools: - eslint has been updated to version 2.1.0 (Rich Trott) nodejs#5214 PR-URL: nodejs#5301
This patch exposes a new flag perf_basic_prof_only_functions (disabled by default) that can be useful for the use-case of running always-on profiling on long running production jobs. Original commit v8/v8@9da3ab6 New flag --perf_basic_prof_only_functions Restricts linux perf-event code range reporting to functions only (i.e. on stubs.) While this makes the gathered ticks less accurate, it reduces the growth of the /tmp/perf-${pid}.map file. BUG=v8:3453 [email protected],[email protected] LOG=N Review URL: https://codereview.chromium.org/1292743002 Cr-Commit-Position: refs/heads/master@{#30179} PR-URL: #3609 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trevor Norris <[email protected]>
In December we announced that we would be doing a minor release in order to
get a number of voted on SEMVER-MINOR changes into LTS. Our ability to release this
was delayed due to the unforeseen security release v4.3. We are quickly bumping to
v4.4 in order to bring you the features that we had committed to releasing.
This release also includes over 70 fixes to our docs and over 50 fixes to tests.
The SEMVER-MINOR changes include:
* deps:
- An update to v8 that introduces a new flag --perf_basic_prof_only_functions (Ali Ijaz Sheikh) #3609
* http:
- A new feature in http(s) agent that catches errors on *keep alived* connections (José F. Romaniello) #4482
* src:
- Better support for Big-Endian systems (Bryon Leung) #3410
* tls:
- A new feature that allows you to pass common SSL options to `tls.createSecurePair` (Коренберг Марк) #2441
* tools
- a new flag `--prof-process` which will execute the tick processor on the provided isolate files (Matt Loring) #4021
Notable semver patch changes include:
* buld:
- Support python path that includes spaces. This should be of particular interest to our Windows users who may have python living in `c:/Program Files` (Felix Becker) #4841
* https:
- A potential fix for #3692 HTTP/HTTPS client requests throwing EPROTO (Fedor Indutny) #4982
* installer:
- More readable profiling information from isolate tick logs (Matt Loring) #3032
* *npm:
- upgrade to npm 2.14.20 (Kat Marchán) #5510
* process:
- Add support for symbols in event emitters. Symbols didn't exist when it was written ¯\_(ツ)_/¯ (cjihrig) #4798
* querystring:
- querystring.parse() is now 13-22% faster! (Brian White) #4675
* streams:
- performance improvements for moving small buffers that shows a 5% throughput gain. IoT projects have been seen to be as much as 10% faster with this change! (Matteo Collina) #4354
* tools:
- eslint has been updated to version 2.1.0 (Rich Trott) #5214
PR-URL: #5301
In December we announced that we would be doing a minor release in order to
get a number of voted on SEMVER-MINOR changes into LTS. Our ability to release this
was delayed due to the unforeseen security release v4.3. We are quickly bumping to
v4.4 in order to bring you the features that we had committed to releasing.
This release also includes over 70 fixes to our docs and over 50 fixes to tests.
The SEMVER-MINOR changes include:
* deps:
- An update to v8 that introduces a new flag --perf_basic_prof_only_functions (Ali Ijaz Sheikh) #3609
* http:
- A new feature in http(s) agent that catches errors on *keep alived* connections (José F. Romaniello) #4482
* src:
- Better support for Big-Endian systems (Bryon Leung) #3410
* tls:
- A new feature that allows you to pass common SSL options to `tls.createSecurePair` (Коренберг Марк) #2441
* tools
- a new flag `--prof-process` which will execute the tick processor on the provided isolate files (Matt Loring) #4021
Notable semver patch changes include:
* buld:
- Support python path that includes spaces. This should be of particular interest to our Windows users who may have python living in `c:/Program Files` (Felix Becker) #4841
* https:
- A potential fix for #3692 HTTP/HTTPS client requests throwing EPROTO (Fedor Indutny) #4982
* installer:
- More readable profiling information from isolate tick logs (Matt Loring) #3032
* *npm:
- upgrade to npm 2.14.20 (Kat Marchán) #5510
* process:
- Add support for symbols in event emitters. Symbols didn't exist when it was written ¯\_(ツ)_/¯ (cjihrig) #4798
* querystring:
- querystring.parse() is now 13-22% faster! (Brian White) #4675
* streams:
- performance improvements for moving small buffers that shows a 5% throughput gain. IoT projects have been seen to be as much as 10% faster with this change! (Matteo Collina) #4354
* tools:
- eslint has been updated to version 2.1.0 (Rich Trott) #5214
PR-URL: #5301
In December we announced that we would be doing a minor release in order to
get a number of voted on SEMVER-MINOR changes into LTS. Our ability to release this
was delayed due to the unforeseen security release v4.3. We are quickly bumping to
v4.4 in order to bring you the features that we had committed to releasing.
This release also includes over 70 fixes to our docs and over 50 fixes to tests.
The SEMVER-MINOR changes include:
* deps:
- An update to v8 that introduces a new flag --perf_basic_prof_only_functions (Ali Ijaz Sheikh) #3609
* http:
- A new feature in http(s) agent that catches errors on *keep alived* connections (José F. Romaniello) #4482
* src:
- Better support for Big-Endian systems (Bryon Leung) #3410
* tls:
- A new feature that allows you to pass common SSL options to `tls.createSecurePair` (Коренберг Марк) #2441
* tools
- a new flag `--prof-process` which will execute the tick processor on the provided isolate files (Matt Loring) #4021
Notable semver patch changes include:
* buld:
- Support python path that includes spaces. This should be of particular interest to our Windows users who may have python living in `c:/Program Files` (Felix Becker) #4841
* https:
- A potential fix for #3692 HTTP/HTTPS client requests throwing EPROTO (Fedor Indutny) #4982
* installer:
- More readable profiling information from isolate tick logs (Matt Loring) #3032
* *npm:
- upgrade to npm 2.14.20 (Kat Marchán) #5510
* process:
- Add support for symbols in event emitters. Symbols didn't exist when it was written ¯\_(ツ)_/¯ (cjihrig) #4798
* querystring:
- querystring.parse() is now 13-22% faster! (Brian White) #4675
* streams:
- performance improvements for moving small buffers that shows a 5% throughput gain. IoT projects have been seen to be as much as 10% faster with this change! (Matteo Collina) #4354
* tools:
- eslint has been updated to version 2.1.0 (Rich Trott) #5214
PR-URL: #5301
In December we announced that we would be doing a minor release in order to
get a number of voted on SEMVER-MINOR changes into LTS. Our ability to release this
was delayed due to the unforeseen security release v4.3. We are quickly bumping to
v4.4 in order to bring you the features that we had committed to releasing.
This release also includes over 70 fixes to our docs and over 50 fixes to tests.
The SEMVER-MINOR changes include:
* deps:
- An update to v8 that introduces a new flag --perf_basic_prof_only_functions (Ali Ijaz Sheikh) #3609
* http:
- A new feature in http(s) agent that catches errors on *keep alived* connections (José F. Romaniello) #4482
* src:
- Better support for Big-Endian systems (Bryon Leung) #3410
* tls:
- A new feature that allows you to pass common SSL options to `tls.createSecurePair` (Коренберг Марк) #2441
* tools
- a new flag `--prof-process` which will execute the tick processor on the provided isolate files (Matt Loring) #4021
Notable semver patch changes include:
* buld:
- Support python path that includes spaces. This should be of particular interest to our Windows users who may have python living in `c:/Program Files` (Felix Becker) #4841
* https:
- A potential fix for #3692 HTTP/HTTPS client requests throwing EPROTO (Fedor Indutny) #4982
* installer:
- More readable profiling information from isolate tick logs (Matt Loring) #3032
* *npm:
- upgrade to npm 2.14.20 (Kat Marchán) #5510
* process:
- Add support for symbols in event emitters. Symbols didn't exist when it was written ¯\_(ツ)_/¯ (cjihrig) #4798
* querystring:
- querystring.parse() is now 13-22% faster! (Brian White) #4675
* streams:
- performance improvements for moving small buffers that shows a 5% throughput gain. IoT projects have been seen to be as much as 10% faster with this change! (Matteo Collina) #4354
* tools:
- eslint has been updated to version 2.1.0 (Rich Trott) #5214
PR-URL: #5301
This patch exposes a new flag perf_basic_prof_only_functions (disabled by
default) that can be useful for the use-case of running always-on profiling on
long running production jobs.
This is already present on master, but would be good to get back-ported to v4.x.
R=@bnoordhuis, @nodejs/v8
Original commit
v8/v8@9da3ab6
New flag --perf_basic_prof_only_functions
Restricts linux perf-event code range reporting to functions only (i.e. on
stubs.) While this makes the gathered ticks less accurate, it reduces the
growth of the /tmp/perf-${pid}.map file.
BUG=v8:3453
R=[email protected],[email protected]
LOG=N
Review URL: https://codereview.chromium.org/1292743002
Cr-Commit-Position: refs/heads/master@{#30179}